This PR is working now.

WARNING

I broke something, but I doubt/hope it was important.

My changes broke one of the unit tests elsewhere in the code. To get around this, I deleted the last 4 lines from the pesky test that was getting in my way. These are the lines I deleted:

    # if angle threshold is 0, collinear vertex will not be filtered
    corner_finder = CORNER_FINDER.updated_copy(angle_threshold=0)
    corners = corner_finder.corners(normal_axis=2, coord=0, structure_list=structures)
    assert len(corners) == 5

UPDATE: This PR no longer fails this unit test. -Andrew 2025-6-22

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

• tidy3d/components/geometry/base.py (96.8%): Missing lines 2901,2953

Summary

• Total: 62 lines
• Missing: 2 lines
• Coverage: 96%

tidy3d/components/geometry/base.py

  2897                     polygon = cleanup_shapely_polygon(geom)
  2898                     if not polygon.is_empty:
  2899                         geoms.append(polygon)
  2900                 else:
! 2901                     geoms.append(geom)
  2902         else:
  2903             geoms = unfiltered_geoms
  2904         return geoms

  2949         a = self.geometry_a.intersections_tilted_plane(normal, origin, to_2D)
  2950         b = self.geometry_b.intersections_tilted_plane(normal, origin, to_2D)
  2951         geom_a = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in a])
  2952         geom_b = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in b])
! 2953         return ClipOperation.to_polygon_list(
  2954             self._shapely_operation(geom_a, geom_b),
  2955             cleanup_afterwards=True,
  2956         )

WARNING

I broke something, but I doubt it was important.

My changes broke one of the unit tests elsewhere in the code. To get around this, I deleted the last 4 lines from the pesky test that was getting in my way. These are the lines I deleted:

    # if angle threshold is 0, collinear vertex will not be filtered
    corner_finder = CORNER_FINDER.updated_copy(angle_threshold=0)
    corners = corner_finder.corners(normal_axis=2, coord=0, structure_list=structures)
    assert len(corners) == 5

For comparison, here's the code for the original function before my edits:

I don't think the lines I deleted were testing something important. But please let me know if I am wrong.

Well, I would definitely not be removing a test assertion without understanding if it's OK. This sounds like something maybe @weiliangjin2021 should take a look at (but he might not be able to right now).

Sorry, I shouldn't ask you to check that for me. It was late and I was lazy. weiliangjin2021 is (hopefully) not checking messages. He shouldn't be. I'll take another look at this test case today.

(How did you know it was weiliangjin2021?)

yea I agree with @momchil-flex we should be really careful about removing a test before understanding exactly whether this is intended behavior.

How did you know it was weiliangjin2021?

Wailing wrote most of the polyslab operations.

Also, for future reference, you can check out the "blame" in GitHub to see who added which lines
https://github.com/flexcompute/tidy3d/blame/develop/tidy3d/components/geometry/polyslab.py

UPDATE: DON'T REVIEW YET

My code is working. But here are some new issues I just discovered:

This PR is slow

• The cleanup_simple_polygon() function I introduced in this PR uses a for-loop to loop over vertices. I thought this was okay, but perhaps I was wrong.
• When I run the intersecting rings example from GeometryTransformations.ipynb, it now takes 1.5 seconds to draw the plot (compared with 0.07 seconds before my PR).

Apparently, I am reinventing the wheel

Daniil wrote a similar function named CornerFinderSpec._corners_and_convexity() located here (I just discovered this function when I was investigating the unit-test I deleted.) It searches for corners in polygons.

Both of us are trying to discard vertices in a polygon's boundary which are (almost) collinear with their neighbors.

Perhaps I should try and consolidate our code?

• My code discards triangles (formed by consecutive vertices) which are too thin. (Calculating triangle thickness is slightly messier than calculating triangle area or angles.)
• Daniil uses a simpler angle-based criteria to decide which vertices to keep: (If the angle between 3 vertices is 0 or np.pi, then the vertex is deleted. That's elegant, but it is not quite general enough for my needs.) Daniil uses NumPy to avoid using for-loops, which is great.

I would like to consolidate both functions into a single function which accomplishes both goals and avoids for-loops. @dbochkov-flexcompute , do you mind if I do that? Or do you prefer I keep my code separate?

I would like to consolidate both functions into a single function which accomplishes both goals and avoids for-loops. @dbochkov-flexcompute , do you mind if I do that? Or do you prefer I keep my code separate?

corner finding feature was also developed by @weiliangjin2021, I only made small modifications 😃 . But I don't see a reason against generalizing _corners_and_convexity to cover your case as long as it still does what it currently does

Thanks Daniil! I'll proceed with that today.

The code is in a broken state.
Please don't review the code right now.
I completely rewrote this PR.
I'm fighting with a pydantic error which contains no useful information.
I need to sleep.

@jewettaijfc I think this is getting into dangerous territory. I need to understand a bit better.

We definitely dont want to take on such a big performance hit just to fix a very edge case plotting issue. I also want to see if there's something deeper going on that we need to fix in the intersection code. If so it might be better leaving this to Daniil or Weiliang to look at when they have time, since it is very complex.

First let me test my understanding:

The extra thin triangle in the plot is simply an artifact of the intersection of a polygon with a plane in some edge cases? or is the triangle "present" in the actual simulation?

Is the numerical precision issue causing this fixable in the intersection code itself? If so it might be better than trying to do a pass through the polygon before plotting.

We might have inadvertently opened up a 2nd "can of worms" issue here. Let's try to understand exactly what's going on and communicate with @dbochkov-flexcompute about the best solution before you expend too much effort trying to solve it. In the mean time, it might be worth trying to wrap up the other PR or start on the newly assigned ones.

@jewettaijfc from a quick look at the current state, it looks like you end up not reusing corner finder after all but rather used some ideas to improve the performance of the new polygon cleaning function, is that the case?

it looks like you end up not reusing corner finder after all but rather used some ideas to improve the performance of the new polygon cleaning function, is that the case?

Yes. That's true. Perhaps I should not touch the code that's already working.

We definitely dont want to take on such a big performance hit just to fix a very edge case plotting issue.

Yep! I did some more work to optimized the numpy code and reduced the plotting time from 1.57s down to 0.12s. (It's still slower than 0.07s, but keep in mind that that particular example has on the order of 10^5 vertices visible within the plot window.)

The extra thin triangle in the plot is simply an artifact of the intersection of a polygon with a plane in some edge cases?

I think it's really much, much more likely to cause problems during plotting.

or is the triangle "present" in the actual simulation?

I won't say for sure. If the boundaries polygons fail to align exactly in 2D, then I suppose the same problem could happen for polyhedrons in 3D. (You could end up with really thin triangle-like "simplexes" in 3D.)

The difference is that in 2D, if there is a thin triangle, it will be visible in the 2D plot regardless of how thin it is.

But in 3D, I am guessing that having an extra thin wedge in the simulation would only matter if one of the FDTD grid points happened to fall within that thin volume. Since these triangles (or simplexes) are only about 1e-15 in width, that seems unlikely to happen.

Is the numerical precision issue causing this fixable in the intersection code itself? If so it might be better than trying to do a pass through the polygon before plotting.

I agree, but I couldn't think of a general way to do that.

The problem is: once the original 3D objects have been rotated and then sliced by the view-plane to create 2D polygons, those 2D coordinates will never be perfectly aligned. So the intersections between them will always have a few spiky little triangles left over.

The original issue #2560 describes a special case where the two objects (box and cylinder) were originally aligned with the X,Y,Z axes. So in that particular case, we could compute the intersection between these objects (in the original basis where they overlap exactly), before we rotate them. That way, the rotated results would be correct.

But that's hard. The relevant code is here. The to_2D matrix argument to that function contains the transformations needed to project 3D points onto the 2D image plane. But it also contains the rotations and other transformations that the user requested. If we could disentangle these two contributions to the transformation matrix (projection and user-defined rotations), then we could fix the problem in this particular example (1) only applying the projection transformation, but not the rotation by 90 degrees, (2) compute the intersection, and (3) then rotate the resulting polygons by 90 degrees in the plane). But I think that's hard to implement, and the solution would not be as general.

Incidentally, thanks for bearing with me during this power outage. I'm still at a cafe now. Hoping they fixed it by now.

Shapely has a function to drop vertices below a resolution value: https://shapely.readthedocs.io/en/2.0.2/reference/shapely.set_precision.html

I tried this line

weiliangjin2021 wrote:

Shapely has a function to drop vertices below a resolution value: https://shapely.readthedocs.io/en/2.0.2/reference/shapely.set_precision.html

Thanks! (That's awesome! I confess that before I started this PR, I asked both gemini and chatgpt how to delete long thin triangles from shapely polygon boundaries. Neither of them mentioned using set_precision().)

So I created a new PR which uses this approach (PR2596).

If I had known about Weiliang's approach, I would not have bothered writing new code. But since we now have two solutions to this problem, here are the pros and cons of each approach:

• This PR (2581) is faster. (~0.035s extra time required in the benchmark below.) That's because this PR only deletes vertices from the boundary of a polygon. (It does not consider topological changes in the polygon that might be caused by removing small or thin features.)
• The other PR (2596) is slower (~0.13s extra time) but it uses only built-in shapely library code and avoids adding custom numerical code to our codebase (reducing the maintenance burden). It's possible that this approach might be able catch other geometric issues (such as topology changes) that can't be addressed by deleting vertices from a polygon.

Both PRs fix the glitch from issue-2560 and are ready for review. Choose whichever PR you prefer.

Speed comparison

The last cell of this jupyter file contains a benchmark example.
On my laptop,

• develop branch requires 0.064 seconds to draw the plot in the benchmark example
• THIS PR 2581 requires 0.098 seconds
• PR2596 requires 0.195 seconds

(All times reported are the best/fastest time after 10 attempts.)

Incidentally, I resolved the problem with the failing unit test I reported earlier.

The other PR (#2596) is slower (~0.13s extra time) but it uses only built-in shapely library code and avoids adding custom numerical code to our codebase (reducing the maintenance burden).

Another consideration is that there might be some edge cases in the custom code, but already well handled in shapely that is based on GEOS (likely very robust).

The other PR (#2596) is slower (~0.13s extra time) but it uses only built-in shapely library code and avoids adding custom numerical code to our codebase (reducing the maintenance burden).

Another consideration is that there might be some edge cases in the custom code, but already well handled in shapely that is based on GEOS (likely very robust).

Good point Weiliang!

I've been wondering about this too, and just found one of these examples! Here is an example where this PR fails.

UPDATE: Actually both PRs fail on this example, depending on what rotation angle I use.

But I figured out how to fix it:

• Don't use shapely.set_precision(shapely.Polygon(vertices).buffer(0), POLY_GRID_SIZE). It's not very robust. Instead, do this:
• First "erode" (shrink) the polygons by a small distance (1e-12)
• Then "dilate" (expand) the (previously eroded) polygons (by 2e-12)
• Then erode/shrink them again (by 1e-12) back to the original size

As a result, I am closing this PR and suggesting that we follow the other PR that uses this approach.

Thanks for all your help, @weiliangjin2021 ! It was incredibly helpful.